fix: address ten reproducible issues from September triage - #1218
Conversation
…ilent When browser setup had already completed but Firefox was simply not running, the bridge stopped responding and every message pointed the agent at action='setup', which is a one-time install/repair flow. Agents then re-ran setup (reopening the extension installer) instead of just starting Firefox. - Detect whether Firefox is running (per-OS) and, when the bridge is installed but silent because Firefox is closed, launch Firefox detached and wait for the bridge to reconnect before failing. - ensure_firefox_ready() now auto-launches Firefox and proceeds with the requested action when the bridge reconnects, prefixing a note. - browser setup and 'jcode browser status'/tool status now distinguish 'Firefox is not running' from a genuinely broken install and say setup is one-time and not needed again. - JCODE_BROWSER_AUTOLAUNCH=0 disables the auto-launch (also used to keep tests hermetic).
|
Scoped review complete. Integrating the ten contribution commits as ancestry-preserving Exact combined executable Fresh named regressions also pass for notification child reaping/missing executable (both paths), real subscribed-client scheduled delivery, idle/busy NotifySession, dynamic MCP schemas/strict-mode exclusion, five real Cargo-cwd cases, all seven file-drop cases, local/SSH skill isolation, tmux terminal recovery, and both actual lock-sensitive tests plus lock-order negative controls. These are Linux/local-fixture checks, not native Ghostty/macOS/Windows or live Kimi acceptance. Full hosted CI has unrelated baseline formatting failures documented in #1193. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Retain an output artifact before publishing the running task or | ||
| // returning its ID. Adopted work buffers its output until completion, | ||
| // when the wrapper below replaces this initially empty file. Create it | ||
| // before spawning that wrapper so a fast completion cannot be truncated. | ||
| let _ = std::fs::write(&output_path, ""); |
There was a problem hiding this comment.
When the configured output directory cannot create the initial artifact, this discarded write error still allows adopt_with_options to publish and return a running task. Callers are then directed to retrieve output but receive None, so background work appears usable while its output is unavailable. Fail adoption before publishing the task, or explicitly expose the initialization failure.
Artifacts
- This authored test holds an adopted task running and compares a writable output root with a file used as an invalid output root, exercising `adopt_with_options` and `output`; it demonstrates the caller-visible missing-output condition.
- This authored script builds the isolated Rust test manifest and captures both control and invalid-output-root executions with command, working directory, and exit code; it provides repeatable proof.
- The executed writable-directory control run exited 0 and shows the returned output file exists while running and `manager_output_while_running=Some("")`; the expected output contract holds.
- The executed invalid-root run exited 0 and shows `adopt_with_options` returned a task while `output_file_exists_while_running=false` and `manager_output_while_running=None`; the defect is confirmed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-base/src/background.rs
Line: 654-658
Comment:
**Reject unavailable output storage**
When the configured output directory cannot create the initial artifact, this discarded write error still allows `adopt_with_options` to publish and return a running task. Callers are then directed to retrieve output but receive `None`, so background work appears usable while its output is unavailable. Fail adoption before publishing the task, or explicitly expose the initialization failure.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| fn reap_notification_child(mut child: std::process::Child) { | ||
| let _ = std::thread::Builder::new() | ||
| .name("jcode-notification-child".to_string()) | ||
| .spawn(move || { | ||
| let _ = child.wait(); | ||
| }); |
There was a problem hiding this comment.
Each notification starts a detached OS thread that blocks in child.wait() until its notifier exits. Repeated notifications with slow or hung notifier processes retain one additional thread each, which can exhaust process resources. This is a non-blocking hardening concern; use bounded child supervision or enforce notifier termination.
Artifacts
- The authored Rust harness preserves the production reaper body, starts controllably long-lived children, and asserts kernel thread growth; it directly tests the reported mechanism.
- The recorded rustc command built the focused harness successfully, establishing that the executable test was ready to run.
- The zero-call baseline executed successfully and recorded one process thread with zero growth, providing the comparison condition.
- The 64-call reproduction executed successfully and recorded 64 started waiters and 64 additional Linux process threads while the children remained alive, confirming unbounded one-per-child growth.
- The captured source excerpt shows the blocking detached reaper at lines 563-568 and its Linux `notify-send` call path, confirming applicability on the validated platform.
- The recorded diff check completed successfully after artifact creation, confirming no whitespace errors were introduced.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-app-core/src/notifications.rs
Line: 563-568
Comment:
**Bound notification reapers**
Each notification starts a detached OS thread that blocks in `child.wait()` until its notifier exits. Repeated notifications with slow or hung notifier processes retain one additional thread each, which can exhaust process resources. This is a non-blocking hardening concern; use bounded child supervision or enforce notifier termination.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| pub(super) fn take_prepared_input(app: &mut App) -> PreparedInput { | ||
| promote_dropped_images(app); | ||
| let raw_input = std::mem::take(&mut app.input); | ||
| app.record_prompt_history(&raw_input); |
There was a problem hiding this comment.
Submitting ordinary keyboard input parses every complete existing local path as a dropped path before preserving the prompt. A typed quoted or shell-escaped filename is sent without its quotes or escapes, changing the text the user entered. This is a non-blocking input-integrity concern; only normalize input with explicit drop or paste provenance.
Artifacts
- Temporary test source copied before cleanup; it types both path forms character-by-character and calls `take_prepared_input`, showing the exact behavior exercised.
- Captured command output for the focused Rust test; both typed forms become the unquoted and unescaped path, confirming normalization is not limited to paste/drop events.
- Captured cleanup command output showing the temporary test changes were reverted from the product source tree.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-tui/src/tui/app/input.rs
Line: 2916-2919
Comment:
**Preserve typed path syntax**
Submitting ordinary keyboard input parses every complete existing local path as a dropped path before preserving the prompt. A typed quoted or shell-escaped filename is sent without its quotes or escapes, changing the text the user entered. This is a non-blocking input-integrity concern; only normalize input with explicit drop or paste provenance.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comments Outside DiffThese findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.
|
1 participant
Summary
Ten scoped fixes from the newest-50 issue triage, with one issue-referencing commit each. Six are new fixes from this pass and four were already present in the local branch base and have been revalidated and explicitly published here. No contributor branch was imported. Unrelated local telemetry, embedding, provider, and workflow changes are excluded.
Review and validation limits
CI follow-through (September 10): the published head has an actual failed CI workflow-validation run, not merely pending tests. GitHub reports
(Line: 16, Col: 1): env is already defined. The Windows smoke and iOS workflow also failed before jobs. These failures are absent from the PR check rollup, where only Require Linked Issue passes. Draft status is not the cause, and marking ready would not repair invalid YAML. The separate CI broken since Aug 20: duplicate top-level env in ci.yml, ios-testflight.yml, windows-smoke.yml (Invalid workflow file) #1191 / fix: safe fixes from September 7 issue triage #1193 workflow repair remains the prerequisite. No branch was imported and no CI failure was bypassed.Draft, not merge-ready. This does not bypass failed or pending required checks. Existing draft fix: safe fixes from September 7 issue triage #1193 owns the separate CI broken since Aug 20: duplicate top-level env in ci.yml, ios-testflight.yml, windows-smoke.yml (Invalid workflow file) #1191 workflow/formatting repair. Its last checked result is 8 passed and 3 failed: Quality Guardrails, Ubuntu Build & Test, and macOS Build & Test. Those changes are not duplicated here. Resolve that baseline CI work and run this branch's full checks before merging.
Exact published snapshot now verified: archived head
0e84b661a1c437b2376398a9be4409b5def6664bwas compiled independently through the host-wide Cargo gate. All 17 focused Rust cohorts listed above passed on the resulting executables, as did all five Cargo workspace tests, job-sizing tests, shell syntax and three source-order regressions. All 1,886 tracked archive files were checked against their published Git blob hashes after testing. This supersedes the initial working-branch-only validation limitation. It is not a full workspace CI run.selfdev testand the coordinated TUI build were blocked before compilation by a pre-existing missing telemetry Git blob. The repository's host-wide Cargo gate was used for local fallback. Remote build host was unavailable, and early high-memory compiler attempts were terminated by earlyoom. Successful focused reruns are the evidence above, not those failed attempts.The first CLI build timed out at ten minutes. A warmed continuation then completed in 2m59s. The exact CLI and a real isolated TUI/daemon both reported
v0.84.0-dev (0e84b661a). The acceptance checks below used that binary with private home/runtime/socket and no inherited API credentials. No shared daemon was reloaded or stopped.Full workspace CI, live Kimi decoding, native macOS/Windows runtime and real SSH host validation remain unverified. The real isolated TUI reached provider selection and reported every provider as not configured. Therefore no live-provider scheduler response is claimed. The scheduler regression uses real sockets with a fake provider, and notification lifecycle tests use instrumented notifier processes. These are narrower evidence, not replacements for the unavailable native/provider acceptance paths.
No issue has been closed and no PR has been merged or released by this triage pass.
Observed public-interface behavior
Desktop notification children still become zombies after the observer-hook fix (#935) #1187: A small caller linked the exact published public notification library. The API returned in 648µs, real
/usr/bin/notify-sendsent the expected notification over the desktop D-Bus, and process tracing confirmed its successfulwait4before the still-alive parent exited. No notifier substitute was used in this check. This is public-library/desktop/OS lifecycle acceptance, not a TUI completion or macOS check.Ghostty: focus-gained handler re-arms mode 1004, causing a 42k writes/s feedback loop in an idle session #1184: In native Ghostty 1.3.1-arch2 on Linux, three focus-away/back cycles reached the exact TUI as actual
focus_gainedevents. The next 5-second focused interval measured 0.20% of one CPU core for Jcode, 1.20% for Ghostty and seven new draw samples. No focus storm recurred in this bounded run. Original focus was restored and the private window/daemon cleaned up. This is not a cross-version or long-duration benchmark.mcp_call arguments silently empty on schema-guided decoders (Kimi K3 via OpenRouter) #1214: With deferred MCP enabled, the exact daemon catalog exposed
mcp_call.arguments.additionalProperties=true. Actual MCP connect/call/disconnect against a controlled local JSON-RPC stdio peer preserved arbitrary keys, nested values, Unicode and escaped quotes in the wire request and result. Owned processes were cleaned up. This is real CLI/daemon/stdio integration with a fixture peer, not live Kimi decoding.Bug: slash autocomplete omits all installed skills despite '18 loaded' header (v0.83.0, Windows) #1186: In the real TUI,
/triage-vplus Tab became/triage-visiblebefore populated History. Captured frames showed the completed skill in the input area with no frame anomalies.Dragging a non-image file leaves a shell-escaped path in the composer that no file tool can open #1206: Enter on a shell-escaped filename produced the unescaped path in real TUI history and its rendered transcript. Passing that exact path to the real daemon read tool returned the file contents.
A hand-written
[sponsors] enabled = falseis undone after one config save and reload #1188: Normal/account default-model ...andclearcommands saved the requested preferences while on-disk[sponsors] enabled = falsesurvived both operations.bg outputfails while a timeout-promoted command is still Running (output artifact created only at completion) #1189: The real daemon Bash tool promoted a 2-second command after 100ms.bg statussaid Running,bg outputsucceeded with an empty artifact, then completion returned exit 0 and the expected command output.The exported
cargoshim silently builds jcode when a Bash command cds into another Rust checkout #1204: Real Cargo metadata after an exported Bash shim, cd and child shell selected the foreign project rather than Jcode. Own workspace/member, symlink and error behavior also passed.Whole-result rerun after requirement mapping
Re-executed all 17 mapped Rust cohorts, five real Cargo workspace tests, job-sizing/shell checks and three lock-order checks against the complete published head. All passed again, and all 1,886 tracked source hashes still matched afterward. The 28-row requirement ledger records each observed output and its evidence boundary, including the still-failing baseline CI prerequisite rather than calling it passed.
Repeated the actual native TUI skill/frame, escaped-path/transcript/read, preference persistence, Bash/background lifecycle and deferred MCP stdio workflows successfully. Repeated native Ghostty focus checks observed actual focus events followed by 0.00% Jcode CPU, 0.40% Ghostty CPU and zero new draw samples over five seconds. Repeated real desktop notification delivery returned from the public API in 7,764µs and reaped the real child before parent exit. Private windows/processes were cleaned up. Test-driver corrections for onboarding dismissal and the correct TOML field were made before the successful complete rerun, without production edits.
Fresh GitHub reads over all 50 initially selected issues reconfirmed one bucket each, 30 newly owner-answered issues, 23 newly labeled issues, all 33 signed comments and ten fix links. All issues remain open. Native Windows/macOS, real SSH and live-provider/Kimi checks remain unverified, and full CI is still blocked. This rerun does not make the PR merge-ready.
Fixes #1184
Fixes #1186
Fixes #1187
Fixes #1188
Fixes #1189
Fixes #1194
Fixes #1201
Fixes #1204
Fixes #1206
Fixes #1214
— Jcode agent (automated triage), on behalf of @1jehuang